Conversation
There was a problem hiding this comment.
Pull request overview
Fixes debug-mode plan JSON round-tripping by preventing an interface-typed field (diff.Diff.Source / DiffSource) from being serialized, which previously made plan.FromJSON() fail when applying --debug plans.
Changes:
- Exclude
diff.Diff.Sourcefrom JSON serialization/deserialization viajson:"-". - Add a regression test that serializes a debug plan with
ToJSONWithDebug(true)and verifiesFromJSON()can load it successfully.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/diff/diff.go |
Stops serializing the interface-typed Diff.Source field to keep debug JSON loadable. |
internal/plan/plan_test.go |
Adds a debug JSON round-trip test covering issue #305. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/diff/diff.go
Outdated
| Operation DiffOperation `json:"operation"` // create, alter, drop, replace | ||
| Path string `json:"path"` | ||
| Source DiffSource `json:"source,omitempty"` | ||
| Source DiffSource `json:"-"` |
There was a problem hiding this comment.
Diff.Source is now excluded from JSON, but the code doesn’t explain why. Consider adding a short inline comment on this field noting that it’s an interface (DiffSource) and is intentionally not serializable/deserializable (issue #305), to reduce the chance of an accidental revert later.
| Source DiffSource `json:"-"` | |
| Source DiffSource `json:"-"` // interface field; intentionally not JSON-serializable/deserializable (see issue #305) |
There was a problem hiding this comment.
Added inline comment: // interface; not JSON-serializable (see #305)
| loaded, err := FromJSON([]byte(debugJSON)) | ||
| if err != nil { | ||
| t.Fatalf("Failed to deserialize debug plan JSON: %v", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Optional: consider asserting that debug mode actually emitted source_diffs (e.g., len(loaded.SourceDiffs) > 0). It makes the test more robust by ensuring ToJSONWithDebug(true) continues to include debug diffs, rather than the test passing if that output changes later.
There was a problem hiding this comment.
Added len(loaded.SourceDiffs) == 0 assertion to verify debug mode actually emits SourceDiffs.
internal/plan/plan_test.go
Outdated
|
|
||
| p := NewPlan(diffs) | ||
|
|
||
| // Serialize with debug mode (includes SourceDiffs with Source interface fields) |
There was a problem hiding this comment.
The comment says debug serialization "includes SourceDiffs with Source interface fields", but with Diff.Source tagged json:"-" the Source field won’t be present in the JSON anymore. Updating the comment to reflect the new behavior will prevent confusion when debugging future failures.
| // Serialize with debug mode (includes SourceDiffs with Source interface fields) | |
| // Serialize with debug mode (includes additional debug metadata but omits Diff.Source interface fields) |
There was a problem hiding this comment.
Fixed. Comment now reads: // includes SourceDiffs; Diff.Source is excluded via json:"-"
Greptile SummaryFixed JSON serialization issue where Key Changes:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 76db096 |
Plans generated with --debug included the Diff.Source field (a DiffSource interface) in JSON output. Go's json.Unmarshal cannot reconstruct interface types, causing FromJSON() to fail when applying debug plans. Change the JSON tag from `json:"source,omitempty"` to `json:"-"` to exclude Source from serialization. All in-memory usage (rewrite, formatting, display) is unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
76db096 to
6c6fb5d
Compare
Summary
--debugincluded theDiff.Sourcefield (aDiffSourceinterface) in JSON output. Go'sjson.Unmarshalcannot reconstruct interface types, causingFromJSON()to fail when applying debug plans viapgschema apply --plan plan.json.Diff.Sourcefromjson:"source,omitempty"tojson:"-"to exclude it from serialization. All in-memory usage (rewrite logic, display formatting) is unaffected.Fixes #305
Test plan
TestPlanDebugJSONRoundTripverifies debug JSON round-trip: serialize withToJSONWithDebug(true)then deserialize withFromJSON()go test -v ./internal/plan -run TestPlanDebugJSONRoundTripgo test -v ./internal/plan🤖 Generated with Claude Code